Skip to content

Conversation

@yaron2
Copy link
Member

@yaron2 yaron2 commented Feb 21, 2025

^ Title

@yaron2 yaron2 marked this pull request as draft February 21, 2025 16:40
@yaron2 yaron2 requested a review from Cyb3rWard0g February 21, 2025 16:57
Signed-off-by: yaron2 <[email protected]>
@yaron2 yaron2 marked this pull request as ready for review February 21, 2025 19:46
Copy link
Contributor

@elena-kolevska elena-kolevska left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the meantime I opened a PR with Conversation API support in the python SDK (dapr/python-sdk#787). If we can get this approved soon I believe it will be easier to just use DaprClient, it handles all kinds of things for us out of the box (tls, retries, healthcheck, all supported env variables...).

@yaron2
Copy link
Member Author

yaron2 commented Feb 24, 2025

In the meantime I opened a PR with Conversation API support in the python SDK (dapr/python-sdk#787). If we can get this approved soon I believe it will be easier to just use DaprClient, it handles all kinds of things for us out of the box (tls, retries, healthcheck, all supported env variables...).

Can we get this is for 1.15?

@elena-kolevska
Copy link
Contributor

Yes, it's last moment, but we're still on time. The PR is ready for review (with full test coverage and docs). I'd like to have at least one more person give it a review and get to an approval.

@yaron2
Copy link
Member Author

yaron2 commented Feb 25, 2025

Updated with the usage of the Dapr Client

Copy link
Contributor

@elena-kolevska elena-kolevska left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just two nits, but overall lgtm

"""
Initializes and returns the Dapr Inference client.
"""
config: DaprInferenceClientConfig = self.config
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The config var doesn't seem to be used.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

prompt_template = Prompty.to_prompt_template(prompty_instance)

# Extract the model configuration from Prompty
model_config = prompty_instance.model
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not used

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@yaron2 yaron2 merged commit c83fe3b into main Feb 26, 2025
@yaron2 yaron2 deleted the convapi-1 branch February 26, 2025 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants